Improve gce bootstrapping in various ways. #740
Conversation
Random-Liu
force-pushed
the
improve-gce-bootstrap
branch
from
0cda57f to
0710b83
Compare
|
/test pull-cri-containerd-node-e2e |
2 similar comments
|
/test pull-cri-containerd-node-e2e |
|
/test pull-cri-containerd-node-e2e |
|
/test pull-cri-containerd-build |
cluster/gce/configure.sh
Outdated
| local -r tmp_env="/tmp/${env_name}.yaml" | ||
| tmp_env_content=$(fetch_metadata "${env_name}") | ||
| if [ -z "${tmp_env_content}" ]; then | ||
| echo "No environment variable is specified in ${env_name}" |
There was a problem hiding this comment.
nit: This might be more clear: Environment variable ${env_name} is not set
There was a problem hiding this comment.
Actually ${env_name} is the name of the env file. I'll make the name less misleading.
There was a problem hiding this comment.
Done. Changed to env_file_name and tmp_env_file.
cluster/gce/configure.sh
Outdated
| fetch_env() { | ||
| local -r env_name=$1 | ||
| ( | ||
| umask 700; |
There was a problem hiding this comment.
Copied from kubernetes code. It seems to be changed recently.
kubernetes/kubernetes#62301
cluster/gce/configure.sh
Outdated
| ) | ||
| } | ||
|
|
||
| # is_preloaded checks whether containerd is preloaded in the image. |
There was a problem hiding this comment.
nit: The function seems generic. Change to whether a package has been preloaded
| deploy_path=${CONTAINERD_DEPLOY_PATH:-"cri-containerd-release"} | ||
| # CONTAINERD_VERSION is the cri-containerd version to use. | ||
| version=${CONTAINERD_VERSION:-""} | ||
| if [ -z "${version}" ]; then |
There was a problem hiding this comment.
nit: Why not just check CONTAINERD_VERSION?
if [ -z "${CONTAINERD_VERSION}" ]; then
There was a problem hiding this comment.
We need :-"" because the env may not be set.
We need version, because we'll use it later anyway.
| exit 1 | ||
| pkg_prefix=${CONTAINERD_PKG_PREFIX:-"cri-containerd-cni"} | ||
| # Behave differently for test and production. | ||
| if [ "${CONTAINERD_TEST:-"false"}" != "true" ]; then |
There was a problem hiding this comment.
Have you verified this code path in a non-test cluster? :-)
There was a problem hiding this comment.
Yeah, actually I only verified the non-test code path. I'll rely on the test-infra to verify the test code path, e.g. the presubmit test has verified node e2e works. :)
| mkdir -p $(dirname ${config_path}) | ||
| cni_bin_dir="${CONTAINERD_HOME}/opt/cni/bin" | ||
| cni_template_path="${CONTAINERD_HOME}/opt/containerd/cluster/gce/cni.template" | ||
| network_policy_provider="${NETWORK_POLICY_PROVIDER:-"none"}" |
There was a problem hiding this comment.
Is NETWORK_POLICY_PROVIDER set in kube-env?
There was a problem hiding this comment.
Yeah, it is from kube-env. Will add comment.
| cni_bin_dir="${CONTAINERD_HOME}/opt/cni/bin" | ||
| cni_template_path="${CONTAINERD_HOME}/opt/containerd/cluster/gce/cni.template" | ||
| network_policy_provider="${NETWORK_POLICY_PROVIDER:-"none"}" | ||
| if [ -n "${network_policy_provider}" ] && [ "${network_policy_provider}" != "none" ] && [ "${KUBERNETES_MASTER:-}" != "true" ]; then |
There was a problem hiding this comment.
Why excluding KUBERNETES_MASTER?
There was a problem hiding this comment.
Because we don't run calico on master. We use kubenet for dockershim, and we use cni template for containerd.
|
@yujuhong Addressed comments. |
|
/lgtm |
Signed-off-by: Lantao Liu <[email protected]>
…arball. Signed-off-by: Lantao Liu <[email protected]>
Signed-off-by: Lantao Liu <[email protected]>
Random-Liu
force-pushed
the
improve-gce-bootstrap
branch
from
fbb7e8a to
d1ba950
Compare
|
New changes are detected. LGTM label has been removed. |
|
Just squash the "Address comments" commit, reapply the lgtm label. |
3 participants
This PR:
containerd-env. We can pass environment variables tocluster/configure.shviacontainerd-envnow.kube-env. We can get kubernetes environment variables fromkube-env.test/configure.shandcluster/gce/configure.sh.NETWORK_POLICY_PROVIDERsupport.Signed-off-by: Lantao Liu [email protected]